rustdoc-json: Output target feature information #139393

willglynn · 2025-04-04T22:49:50Z

#[target_feature] attributes refer to a target-specific list of features. Enabling certain features can imply enabling other features. Certain features are always enabled on certain targets, since they are required by the target's ABI. Features can also be enabled indirectly based on other compiler flags.

Feature information is ultimately known to rustc. Rather than force external tools to track it – which may be wildly impractical due to -C target-cpu – have rustdoc output rustc's feature data.

This change is motivated by obi1kenobi/cargo-semver-checks#1246, which intends to detect semver hazards caused by #[target_feature].

rustbot · 2025-04-04T22:49:54Z

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-04-07T18:23:19Z

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

willglynn · 2025-04-07T18:23:21Z

I'm unsure about two topics here:

Does adding a field count as a backwards incompatible change for rustdoc-json-types purposes? I decided no due to the absence of #[serde(deny_unknown_fields)], so I did not increment FORMAT_VERSION.
Is it acceptable to output information about unstable target features from a stable toolchain? I decided yes so I unconditionally output both stable and unstable user-specifiable target features, and I decided that the stability distinction was important so I provide the associated language feature for any unstable target features.

Besides stable and unstable, there is also rustc_target::target_features::Stability::Forbidden:

    /// This feature can not be set via `-Ctarget-feature` or `#[target_feature]`, it can only be
    /// set in the target spec. It is never set in `cfg(target_feature)`. Used in
    /// particular for features are actually ABI configuration flags (not all targets are as nice as
    /// RISC-V and have an explicit way to set the ABI separate from target features).
    Forbidden { reason: &'static str },

I decided forbidden features are effectively internal implementation details (unreachable by the user) and so I drop them from the list.

Enselic · 2025-04-08T03:03:44Z

A new struct field is backwards compatible (edit: if it's Option or uses something like #[serde(default)] but we probably want neither in this case) but also a new FORMAT_VERSION.

willglynn · 2025-04-08T03:10:22Z

Got it. Is there a better way to phrase this requirement, and if so, would it belong in this PR?

rust/src/rustdoc-json-types/lib.rs

Lines 28 to 33 in 58fd7b4

    
           /// The version of JSON output that this crate represents. 
        
           /// 
        
           /// This integer is incremented with every breaking change to the API, 
        
           /// and is returned along with the JSON blob as [`Crate::format_version`]. 
        
           /// Consuming code should assert that this value matches the format version(s) that it supports. 
        
           pub const FORMAT_VERSION: u32 = 44;

Enselic · 2025-04-08T03:34:30Z

Looking closer at your code I think this is actually a breaking change since the new field is not Option and does not use something like #[serde(default)] but I don't think we want anything like that in this case.

To be clear: I expect JSON from an old nightly to fail to deserialize into the new struct, but I expect JSON from a new nightly to successfully deserialize into the old struct.

willglynn · 2025-04-08T03:41:36Z

Oh, sure, I was thinking about the old-reading-new case and not the new-reading-old case. You're absolutely right that this is a breaking change.

aDotInTheVoid · 2025-04-08T21:06:26Z

Does adding a field count as a backwards incompatible change for rustdoc-json-types purposes? I decided no due to the absence of #[serde(deny_unknown_fields)], so I did not increment FORMAT_VERSION.

Yep, FORMAT_VERSION needs to be bumped here. We should probably write down the rules in more detail somewhere.

Is it acceptable to output information about unstable target features from a stable toolchain?

rustdoc-json is an unstable feature, and like all unstable features is not available/supported on a stable toolchain.

aDotInTheVoid · 2025-04-08T21:06:33Z

r? @aDotInTheVoid

aDotInTheVoid

Thanks for working on this! I think the overall approach makes sense.

This needs to have some tests, which will be in the src/rustdoc-json suite. It's not yet documented, sorry. It's probably best to start by looking at some examples.

src/librustdoc/json/mod.rs

aDotInTheVoid · 2025-04-08T20:54:06Z

src/librustdoc/json/mod.rs

+        types::Target {
+            target_features: sess
+                .target
+                .rust_target_features()


There should be a comment why you need to use this and not tcx.rust_target_features. Rustdoc kinda fakes what's going on with target features because it wants to run for multiple targets. See #137632 for details.

src/librustdoc/json/mod.rs

src/rustdoc-json-types/lib.rs

willglynn · 2025-04-09T00:32:45Z

I have a tests/rustdoc-json/target.rs which passes and spot checks the output on my machine:

// Target tests are necessarily target-specific

// aarch64-*-*:
//   `sve2` is stable and implies `sve`
//@[aarch64] is "$.target.target_features[?(@.name=='sve2')].unstable_feature_gate" null
//@[aarch64] has "$.target.target_features[?(@.name=='sve2')].implies_features[*]" \"sve\"

// aarch64-apple-darwin:
//   We have the right triple, and `sve2` is enabled by default
//@[aarch64-apple-darwin] is "$.target.triple" \"aarch64-apple-darwin\"
//@[aarch64-apple-darwin] is "$.target.target_features[?(@.name=='sve2')].globally_enabled true

I'll add similar coverage for other tier 1 targets.

rustbot · 2025-04-09T04:11:00Z

Some changes occurred in tests/rustdoc-json

cc @aDotInTheVoid

Some changes occurred in src/tools/compiletest

cc @jieyouxu

willglynn · 2025-04-09T04:18:09Z

It turns out that jsondocck doesn't support //@[target] so I ended up using compiletest's //@ only-[target] support to enable one test file for each tier 1 target. I also included targets/{aarch64,x86_64}_reflects_compiler_options.rs tests which use //@ compile-flags: -Ctarget-feature to globally enable a target feature and assert that this changes the output in expected ways.

aDotInTheVoid

This looks really close. A couple more things that still need tests.

implies_features and unstable_feature_gate fields
That we dont have features from other architectures. e.g. x86 doesn't have sve, and aarch64 doesn't have avx. (Because rustdoc sometimes does spooky stuff here)

These tests don't need to be super exhaustive across a wide variety of archetectures, as that doesn't actually get us any more coverage (remember, we don't need to test that rustc has correct target features, only that rustdoc correctly conveys it), but does increase the maintenence burden.

@rustbot author

aDotInTheVoid · 2025-04-09T22:10:20Z

src/tools/jsondocck/src/main.rs

@@ -142,6 +142,9 @@ impl CommandKind {
            (_, false) if KNOWN_DIRECTIVE_NAMES.contains(&command_name) => {
                return None;
            }
+            (name, _) if name.starts_with("only-") => {


Is this also needed given the updates to the KNOWN_DIRECTIVE_NAMES list?

Yes, due to command parsing differences between the tools. The line //@ only-aarch64-apple-darwin otherwise causes:

Invalid command `//@ only-aarch`

src/rustdoc-json-types/lib.rs

rustbot · 2025-04-09T22:19:47Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

`#[target_feature]` attributes refer to a target-specific list of features. Enabling certain features can imply enabling other features. Certain features are always enabled on certain targets, since they are required by the target's ABI. Features can also be enabled indirectly based on other compiler flags. Feature information is ultimately known to `rustc`. Rather than force external tools to track it -- which may be wildly impractical due to `-C target-cpu` -- have `rustdoc` output `rustc`'s feature data.

willglynn · 2025-04-10T03:56:35Z

@aDotInTheVoid Thanks! I rebased, and added tests to exercise the other two fields and verify non-existence of a target feature for each tier 1 target.

@rustbot ready

rustbot assigned notriddle Apr 4, 2025

rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 4, 2025

This comment has been minimized.

Sign in to view

willglynn force-pushed the rustdoc_output_target_feature_information branch 2 times, most recently from 7c6a12b to cb2b075 Compare April 7, 2025 17:30

willglynn marked this pull request as ready for review April 7, 2025 18:23

willglynn force-pushed the rustdoc_output_target_feature_information branch from cb2b075 to 58fd7b4 Compare April 8, 2025 03:06

aDotInTheVoid changed the title ~~rustdoc: Output target feature information~~ rustdoc-json: Output target feature information Apr 8, 2025

rustbot assigned aDotInTheVoid and unassigned notriddle Apr 8, 2025

aDotInTheVoid reviewed Apr 8, 2025

View reviewed changes

willglynn force-pushed the rustdoc_output_target_feature_information branch from 58fd7b4 to 4ccc3c0 Compare April 8, 2025 22:54

This comment has been minimized.

Sign in to view

willglynn force-pushed the rustdoc_output_target_feature_information branch from 4ccc3c0 to 5cc3718 Compare April 9, 2025 04:10

rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 9, 2025

willglynn force-pushed the rustdoc_output_target_feature_information branch from 5cc3718 to 83b1504 Compare April 9, 2025 04:20

This comment has been minimized.

Sign in to view

willglynn force-pushed the rustdoc_output_target_feature_information branch from 83b1504 to 68c9794 Compare April 9, 2025 12:59

aDotInTheVoid reviewed Apr 9, 2025

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 9, 2025

willglynn force-pushed the rustdoc_output_target_feature_information branch from 68c9794 to 0632832 Compare April 10, 2025 02:46

willglynn force-pushed the rustdoc_output_target_feature_information branch from 0632832 to 4ecdefd Compare April 10, 2025 02:50

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc-json: Output target feature information #139393

rustdoc-json: Output target feature information #139393

willglynn commented Apr 4, 2025

rustbot commented Apr 4, 2025

This comment has been minimized.

rustbot commented Apr 7, 2025

willglynn commented Apr 7, 2025

Enselic commented Apr 8, 2025 •

edited

Loading

willglynn commented Apr 8, 2025

Enselic commented Apr 8, 2025 •

edited

Loading

willglynn commented Apr 8, 2025

aDotInTheVoid commented Apr 8, 2025

aDotInTheVoid commented Apr 8, 2025

aDotInTheVoid left a comment

aDotInTheVoid Apr 8, 2025

This comment has been minimized.

willglynn commented Apr 9, 2025

rustbot commented Apr 9, 2025

willglynn commented Apr 9, 2025

This comment has been minimized.

aDotInTheVoid left a comment

aDotInTheVoid Apr 9, 2025

willglynn Apr 10, 2025

rustbot commented Apr 9, 2025

willglynn commented Apr 10, 2025

rustdoc-json: Output target feature information #139393

Are you sure you want to change the base?

rustdoc-json: Output target feature information #139393

Conversation

willglynn commented Apr 4, 2025

rustbot commented Apr 4, 2025

This comment has been minimized.

rustbot commented Apr 7, 2025

willglynn commented Apr 7, 2025

Enselic commented Apr 8, 2025 • edited Loading

willglynn commented Apr 8, 2025

Enselic commented Apr 8, 2025 • edited Loading

willglynn commented Apr 8, 2025

aDotInTheVoid commented Apr 8, 2025

aDotInTheVoid commented Apr 8, 2025

aDotInTheVoid left a comment

Choose a reason for hiding this comment

aDotInTheVoid Apr 8, 2025

Choose a reason for hiding this comment

This comment has been minimized.

willglynn commented Apr 9, 2025

rustbot commented Apr 9, 2025

willglynn commented Apr 9, 2025

This comment has been minimized.

aDotInTheVoid left a comment

Choose a reason for hiding this comment

aDotInTheVoid Apr 9, 2025

Choose a reason for hiding this comment

willglynn Apr 10, 2025

Choose a reason for hiding this comment

rustbot commented Apr 9, 2025

willglynn commented Apr 10, 2025

Enselic commented Apr 8, 2025 •

edited

Loading

Enselic commented Apr 8, 2025 •

edited

Loading